Skip to content
This repository was archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-532. Use Notebook CRD & controller to spawn jupyter notebook#335

Closed
lowc1012 wants to merge 2 commits into
apache:masterfrom
lowc1012:SUBMARINE-532
Closed

SUBMARINE-532. Use Notebook CRD & controller to spawn jupyter notebook#335
lowc1012 wants to merge 2 commits into
apache:masterfrom
lowc1012:SUBMARINE-532

Conversation

@lowc1012
Copy link
Copy Markdown
Contributor

What is this PR for?

This PR shows how to use Kubeflow's Notebook CRD & controller to spawn jupyter notebook

What type of PR is it?

[Improvement]

Todos

  1. User doc of notebook and example yaml

What is the Jira issue?

SUBMARINE-532

How should this be tested?

CI

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@tangzhankun
Copy link
Copy Markdown
Contributor

@lowc1012 Thanks for the contribution! One question, will the notebook controller's YAML files be added to submarine helm charts? The current script based on "kind" is for testing, right?

@lowc1012
Copy link
Copy Markdown
Contributor Author

lowc1012 commented Jul 1, 2020

@tangzhankun
Yes. After the PR is merged, I will create another PR for deploying NB-controller by using helm charts.
And yes, it is added into submarine test-k8s.

@lowc1012 lowc1012 changed the title SUBMARINE-532. [WIP] Use Notebook CRD & controller to spawn jupyter notebook SUBMARINE-532. Use Notebook CRD & controller to spawn jupyter notebook Jul 1, 2020
@pingsutw
Copy link
Copy Markdown
Member

pingsutw commented Jul 2, 2020

/cc @liuxunorg @jiwq

Copy link
Copy Markdown

@aylei aylei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest LGTM

# See the License for the specific language governing permissions and
# limitations under the License.
#
set -e
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about set -euo pipefail

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!
many shell scripts need to be set to set -euo pipefail.
Fix this problem in the next PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aylei Thanks for your review!
@liuxunorg Ok, I will fix other shell scripts too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lowc1012 Thanks!

@xunliu
Copy link
Copy Markdown
Member

xunliu commented Jul 2, 2020

Will merge if no more comments.

@asfgit asfgit closed this in 3c285b0 Jul 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants